Repository navigation
Conversation
|
Claude finished @jcastro-dotcms's task in 2m 49s —— View job Code ReviewReviewed the diff against Existing (prior findings still present)
Notes (verified correct, not findings)
No new critical/high issues. The flag-off behavior is byte-for-byte unchanged. · branch |
9690354 to
5ffcec2
Compare
003b2a4 to
d55984c
Compare
|
Pull Request Unsafe to Rollback!!!
|
d55984c to
d3e4707
Compare
5ffcec2 to
00cb9e1
Compare
00cb9e1 to
a7e7291
Compare
d3e4707 to
65be43b
Compare
65be43b to
95368f9
Compare
a7e7291 to
187a09b
Compare
95368f9 to
ca2b3b5
Compare
187a09b to
9422974
Compare
ca2b3b5 to
9bd3fa8
Compare
9422974 to
0b1ed27
Compare
nollymar
added
the
PR : dotbot review
…3 asset storage Final slice of the S3 asset storage work. With FEATURE_FLAG_S3_ASSET_STORAGE on, completed image-filter outputs and compiled Sass are stored in the generated-assets group and restored cold without recomputation, rendition keys include the original's revision, crops use the snapshot focal point, and Markdown, VTL and included files open through the binary asset API so evicted files are restored before rendering. Flag-off rendering matches main.
…lable when S3 misbehaves With S3 asset storage on, the JPEG filter now predicts the .jpg file it writes, so warm JPEG renditions are found on local disk instead of making S3 requests on every request and failing during an S3 outage. With the flag off the inherited .png prediction is kept, so flag-off cache paths are unchanged. When a filter returns its input unchanged (for example a maximum width larger than the image), the exporter remembers on that node that the predicted output is never produced, so later requests skip the S3 lookup for it, both per filter and for the chain's final output. Only an output written at its predicted path is uploaded, and the upload now runs after the image permit is released. A failed rendition or compiled CSS upload no longer fails the request. The locally produced output is served, a warning is logged, and the file stays local only, which eviction already never removes. CSSAssetStorageTest now asserts this policy instead of the old fail-closed one. The Sass compiler no longer holds the cache lease while Dart Sass runs; it holds it only while copying sources and while reading a stored output. The rendition lease still spans the whole export and is documented as a known limitation. Focal-point reads treat a permission denial or missing content as no focal point again, as with the flag off, so $content.image.fpx keeps working for anonymous visitors; only a DotDataException from the lookup is rethrown. DotResourceLoader reports a storage failure as a non-cached VelocityException only for the file-backed loaders (VTL, macros, legacy VL and includes). Container, template, page and other database loaders keep the cached ResourceNotFoundException. VTLLoader and IncludeLoader only continue to the restore path for a missing file under the asset root. A missing file elsewhere is a plain not-found again, instead of a "POSSIBLE HACK ATTACK" warning. Adds unit tests for the JPEG prediction, warm JPEG and no-op renditions during an outage, a failed rendition upload, focal-point lookups and the Velocity loader failure handling, and updates BINARY_S3_STORAGE.md.
0b1ed27 to
e7d3e70
Compare
9bd3fa8 to
0eedddc
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| throws BinaryContentExporterException { | ||
| if (s3Renditions()) { | ||
| try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) { | ||
| return exportContentInternal(file, suppliedParameters); |
There was a problem hiding this comment.
🟡 [P2] ImageFilterExporter.java:76 eviction lease held across full filter chain
Current code:
if (s3Renditions()) {
try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) {
return exportContentInternal(file, suppliedParameters);
}
}Problem: The process-wide eviction read-lock (BinaryAssetStorageAPIImpl.acquireCacheLease() → evictionLock.readLock()) is held for the entire export: pixel work of every filter, S3 downloads/uploads, and the image-semaphore wait. This starves the evictor's write lock for the duration of all concurrent image renders, and is redundant because getGeneratedFile/storeGeneratedFile/openLocalFile already acquire the lease internally. The CSS path in this same PR deliberately scopes leases to source copies and stored-output reads only.
Fix:
return exportContentInternal(file, suppliedParameters);and rely on the per-call leases already taken inside cachedRendition/storeRendition.
| if (!com.dotcms.storage.AssetStorageFeature.isEnabled()) { | ||
| return APILocator.getFileAssetAPI().getRealAssetPath(conInode, file.getUnderlyingFileName()); | ||
| } | ||
| final File binaryFile = APILocator.getBinaryAssetStorageAPI() |
There was a problem hiding this comment.
🟡 [P2] WebAPI.java:408 getAssetPath returns null where flag-off path always returns a path
Current code:
final File binaryFile = APILocator.getBinaryAssetStorageAPI()
.getBinaryFile(conInode, FileAssetAPI.BINARY_FIELD);
return binaryFile != null ? binaryFile.getAbsolutePath() : null;Problem: With the flag on, getBinaryFile(inode, field) returns null when the row's contentlet_as_json records no binary for the field or the restore misses, while the flag-off path returns the legacy asset path unconditionally. Templates calling $webapi.getAssetPath(...) that previously received a path can now get null.
Fix:
return binaryFile != null ? binaryFile.getAbsolutePath()
: APILocator.getFileAssetAPI().getRealAssetPath(conInode, file.getUnderlyingFileName());Assumption: rows whose JSON lacks the fileAsset field are reachable here. What to verify: that getBinaryFile cannot return null for a FileAsset already resolved via fromContentlet above.
|
|
||
| final FileAsset mainFile = APILocator.getFileAssetAPI() | ||
| .fromContentlet(APILocator.getContentletAPI().find(info.get().getWorkingInode(), | ||
| .fromContentlet(APILocator.getContentletAPI().find( |
There was a problem hiding this comment.
⚪ [P3] DotLibSassCompiler.java:106 null live inode breaks live compile when live version is absent
Current code:
.fromContentlet(APILocator.getContentletAPI().find(
com.dotcms.storage.AssetStorageFeature.isEnabled() && live
? info.get().getLiveInode() : info.get().getWorkingInode(),Problem: With the flag on and live == true, a null getLiveInode() makes find(null, …) throw, failing a request that compiled from the working inode before.
Fix:
final String inode = com.dotcms.storage.AssetStorageFeature.isEnabled() && live
&& info.get().getLiveInode() != null
? info.get().getLiveInode() : info.get().getWorkingInode();Assumption: CSSPreProcessServlet confirms a live asset for the identifier before compiling, so getLiveInode() should be non-null in practice. What to verify: that a published-then-unpublished SCSS file cannot reach this line in live mode; the null-guard removes the risk regardless.
|
dotbot code review:
All new S3 paths are gated on AssetStorageFeature with legacy fallback preserved. No provable P0/P1 regression introduced by this patch was found in the rendition, CSS, VTL and focal-point flows. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · meta/muse-spark-1.3 · medium |
|
|
||
| final FileAsset mainFile = APILocator.getFileAssetAPI() | ||
| .fromContentlet(APILocator.getContentletAPI().find(info.get().getWorkingInode(), | ||
| .fromContentlet(APILocator.getContentletAPI().find( |
There was a problem hiding this comment.
⚪ [P3] DotLibSassCompiler.java:107 null live inode breaks live compile when live version is absent
Current code:
.fromContentlet(APILocator.getContentletAPI().find(
com.dotcms.storage.AssetStorageFeature.isEnabled() && live
? info.get().getLiveInode() : info.get().getWorkingInode(),Problem: With flag on and live, a null getLiveInode() makes find(null, …) throw, failing requests that previously compiled from the working inode.
Fix:
.fromContentlet(APILocator.getContentletAPI().find(
com.dotcms.storage.AssetStorageFeature.isEnabled() && live
&& info.get().getLiveInode() != null
? info.get().getLiveInode() : info.get().getWorkingInode(),Assumption: a live asset is normally confirmed before compiling, so this is defensive. What to verify: that a published-then-unpublished SCSS file cannot reach this line in live mode.
| if (!com.dotcms.storage.AssetStorageFeature.isEnabled()) { | ||
| return APILocator.getFileAssetAPI().getRealAssetPath(conInode, file.getUnderlyingFileName()); | ||
| } | ||
| final File binaryFile = APILocator.getBinaryAssetStorageAPI() |
There was a problem hiding this comment.
🟡 [P2] WebAPI.java:410 getAssetPath returns null where flag-off path always returns a path
Current code:
final File binaryFile = APILocator.getBinaryAssetStorageAPI()
.getBinaryFile(conInode, FileAssetAPI.BINARY_FIELD);
return binaryFile != null ? binaryFile.getAbsolutePath() : null;Problem: Flag-on returns null when the JSON row lacks the binary field; flag-off always returns a path, changing the template contract.
Fix:
return binaryFile != null ? binaryFile.getAbsolutePath()
: APILocator.getFileAssetAPI().getRealAssetPath(conInode, file.getUnderlyingFileName());Assumption: rows without a recorded binary are reachable here. What to verify: that getBinaryFile cannot return null for a FileAsset resolved via fromContentlet above.
| public BinaryContentExporterData exportContent(File file, final Map<String, String[]> parameters) | ||
| public BinaryContentExporterData exportContent(File file, final Map<String, String[]> suppliedParameters) | ||
| throws BinaryContentExporterException { | ||
| if (s3Renditions()) { |
There was a problem hiding this comment.
🟡 [P2] ImageFilterExporter.java:75 eviction lease held across full filter chain
Current code:
if (s3Renditions()) {
try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) {
return exportContentInternal(file, suppliedParameters);
}
}Problem: Process-wide eviction read-lock is held across all filters' pixel work, S3 I/O, and semaphore waits, starving the evictor's write lock. getGeneratedFile/storeGeneratedFile/openLocalFile already take leases internally, so this is redundant.
Fix:
return exportContentInternal(file, suppliedParameters);The PR doc acknowledges this is a known follow-up, so tracking it here to ensure it is not lost.
|
dotbot code review:
All new S3 asset-storage behavior is gated on AssetStorageFeature with legacy fallbacks when the flag is off, so default deployments are unaffected. The remaining issues are flag-on edge cases (null live inode, null asset-path return, and the acknowledged full-export eviction lease), not provable P0/P1 regressions. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · ~z-ai/glm-latest · medium |
This branch has not been deployed
3 participants
Refs #37868
Proposed Changes
generated-assetsand restored cold without re-running the filter; warm hits never contact S3, including JPEG output and filters that return their input unchanged. Rendition keys include the original's revision, so a replacement never serves the old rendition. Crops use the focal point from the content's metadata snapshot, with the Java and native engines pinned to the same coordinates. If uploading a produced rendition fails, the rendition is still served and stays local only; eviction never removes a file without a durable copy.Behavior with the flag off
Unchanged from main; renditions keep the existing two-character layout and cache paths.
Review fixes
The last commit on this branch (
fix(storage): keep renditions, compiled CSS and template loading available when S3 misbehaves) addresses a full review of this PR. All of it is flag-on only:JpegImageFilterpredicted a.pngresult while writing a.jpg, so JPEG renditions (the default Java engine) looked up S3 on every request, never restored from S3, and failed during an S3 outage. Filters that return their input unchanged had the same effect. The exporter now remembers, per node and bounded, predicted outputs that a no-op filter never writes, and uploads an output only when it was written at its predicted path, after the image permit is released.$content.image.fpx,fpyandfocalPointwork again for anonymous visitors: a permission denial or missing content means "no focal point", as on main, and only a storage failure is rethrown.Additional Info
With this PR merged, the stack carries the complete feature.
Checklist
ImageFilterExporterStorageTest,FocalPointAPIImplTestandStoredVelocityFileLoaderTest.BinaryAssetStorageIntegrationTest,ContentletBackupStorageTest,SharedAssetStorageIntegrationTest,BinaryAssetStarterRestoreTest,PublishingArchiveStorageTest,DotWebdavHelperTest,AssetTemplateStorageTest,CSSAssetStorageTest,ContentFileAssetIntegrityCheckerTest) and the existingContentletAPITest,FileMetadataAPITest,TempFileAPITest,BinaryCleanupJobTest,BinaryExporterServletTest,FocalPointAPITestandBundleResourceTest. Three cases ofContentletAPITest.testCheckin_nullRequiredFieldValuefailed on the first pass and passed on rerun. The test saves a content type under a fixed variable while the previous case's content type is still being deleted asynchronously, and the stack changes no content-type code.ContentletAPITest: 141 run, 1 failure. The failure was a test from 2 that still expected the servlet to hold its lease for the whole response; after updating it to expect the lease to end once the file is open, that class passed 14 of 14 on rerun.